-
Notifications
You must be signed in to change notification settings - Fork 844
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really clear and easy to follow changes, good job! :)
Just left a couple of comments, otherwise LGTM
// --all-features -- --nocapture` | ||
// The value rows_range_chip_table has been optained by patching the halo2 | ||
// library to report the number of rows used in the range chip table | ||
// region. TODO: Figure out a way to get these numbers automatically. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good workaround! We should open an issue to address this once we merge the PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could min_num_rows_block
end up as a trait-fn
associated to the SubCircuit
trait or something similar?? I think it would make much more sense than having it as we have it here. Specially now that #937 is merged and integrated cross all the circuits.
// --all-features -- --nocapture` | ||
// The value rows_range_chip_table has been optained by patching the halo2 | ||
// library to report the number of rows used in the range chip table | ||
// region. TODO: Figure out a way to get these numbers automatically. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @ed255 just pinging you to give visibility on CPerezz comment's before holiday :) |
36f6040
to
6562e83
Compare
Add two new unit tests for the super circuit: - block with 1 tx with max_txs=2 - block with 2 txs with max_txs=2 The super circuit unit tests are still marked as ignored so that they are skipped in the github PR checks because they take a long time to run Improve ergonomics of rows estimation per circuit. Add a function `min_num_rows_block` to each circuit (via the SubCircuit trait) to get the minimum number of required rows to prove a given block. Related to #982 Resolve #883
6562e83
to
8f934de
Compare
This is a great idea! It looks better organized this way :) And I actually found that I forgot to add this function to the PiCircuit! I've applied this in the last PR update. |
I think I've addressed all comments from @davidnevadoc and @CPerezz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just left a comment. Address it if you consider.
* temporarily comment out rlc * correct modulo and ec mul * correct import * rlc from word limbs: * fix ec mul * ec mul adapt rlc pattern * change ecrecover structs * correct parts of ecrecover gadget * fix modulo conflict * fix precompile failed error state * remove import * fix precompile gadget * fit lt_word * remove import * remove import * remove import * fix callop * remove import * fix modulo * restore rlc * minor math fix * fix lt generic * remove lt word cell type * fix lt word gadget related code * fix modulo * fix ecrecover constants * remove import --------- Co-authored-by: DreamWuGit <[email protected]>
Add two new unit tests for the super circuit:
Improve ergonomics of rows estimation per circuit. Add a function
min_num_rows_block
to each circuit to get the minimum number of required rows to prove a given block. Related to #982Resolve #883